feat(spec-specs): EIP-7708 ETH Transfers Emit a Log#2023
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## eips/amsterdam/eip-7708 #2023 +/- ##
===========================================================
+ Coverage 86.07% 86.14% +0.07%
===========================================================
Files 599 599
Lines 39527 39491 -36
Branches 3780 3782 +2
===========================================================
Hits 34021 34021
+ Misses 4872 4848 -24
+ Partials 634 622 -12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
d2ee487 to
dd32088
Compare
spencer-tb
left a comment
There was a problem hiding this comment.
Thanks for getting the ball rolling! I've been looking at the spec, discussions in magicians & EIPs#9003, and I think we should target a minimal spec for devnet-2. The same as EIPs#9003 but without fee payments/withdrawals:
So we have these 3 triggering the log:
- Any nonzero-value-transferring transaction, before any other logs created by EVM execution
- Any nonzero-value CALL, at the time that the value transfer executes
- Any nonzero-value-transferring SELFDESTRUCT, at the time that the value transfer executes
Using (as in EIPs#9003):
SYSTEM_ADDRESS = 0xfffffffffffffffffffffffffffffffffffffffetopics[0] = keccak256('Transfer(address,address,uint256)')
Leaving the following for future discussion and devnets:
- System address & magic values (
topics[0]) - Fee payment logs
- Withdrawal logs
Given the above please see comments below.
Ohh and can we make this target an ethereum/eips/amsterdam/eip-7708 branch?
| from ..trie import Trie | ||
|
|
||
| __all__ = ("Environment", "Evm", "Message") | ||
| MAGIC_XACTION_LOG_HASH = keccak256(b"42") |
There was a problem hiding this comment.
| MAGIC_XACTION_LOG_HASH = keccak256(b"42") | |
| TRANSFER_TOPIC = keccak256(b"Transfer(address,address,uint256)") | |
| SYSTEM_ADDRESS = Address(bytes.fromhex("fffffffffffffffffffffffffffffffffffffffe")) |
| transfer_log( | ||
| evm, | ||
| message.current_target, | ||
| message.caller, | ||
| message.value, | ||
| ) |
There was a problem hiding this comment.
I think execute_code runs for every contract, including nested CALLs, so CALL logs once in system.py, then again here.
| transfer_log( | |
| evm, | |
| message.current_target, | |
| message.caller, | |
| message.value, | |
| ) |
Following some chats with claude, I think we should put this in process_message_call, inside the success branch:
if message.value > 0: # nonzero-value-transferring transaction from spec
padded_sender = left_pad_zero_bytes(message.caller, 32)
padded_recipient = left_pad_zero_bytes(message.current_target, 32)
tx_transfer_log = Log(
address=SYSTEM_ADDRESS,
topics=(
TRANSFER_TOPIC,
Hash32(padded_sender),
Hash32(padded_recipient),
),
data=message.value.to_be_bytes32(),
)
logs = (tx_transfer_log,) + logsWith the latter we log for "nonzero-value-transferring transactions" and do not duplicate logs for CALL.
There was a problem hiding this comment.
Ideally, calls to emit_transfer_log() should go next to calls to move_ether(). There two calls to move_ether(), in selfdestruct() and in process_message(). In selfdestruct() the situation is straightforward.
Unfortunately process_message() doesn't have access to the EVM object, because it is created in execute_code(). I think the best solution is the inline execute_code() into process_message(). No one else calls execute_code() and I can see no reason for the two functions to be distinct.
|
@Carsons-Eels @spencer-tb just a heads up, I just created a new target branch |
|
Tagging the EIPs PR: ethereum/EIPs#9003 |
petertdavies
left a comment
There was a problem hiding this comment.
I've left a few comments. My opinions on proposed changes to the EIP:
- Log address change: Sensible and uncontroversial
- Topic change: Good idea and uncontroversial
- Withdrawals: This has been deferred to a future EIP for sound technical reasons
- Fee logs: This needs to be dealt with in the EIPs repo, before getting merged here.
| transfer_log( | ||
| evm, | ||
| message.current_target, | ||
| message.caller, | ||
| message.value, | ||
| ) |
There was a problem hiding this comment.
Ideally, calls to emit_transfer_log() should go next to calls to move_ether(). There two calls to move_ether(), in selfdestruct() and in process_message(). In selfdestruct() the situation is straightforward.
Unfortunately process_message() doesn't have access to the EVM object, because it is created in execute_code(). I think the best solution is the inline execute_code() into process_message(). No one else calls execute_code() and I can see no reason for the two functions to be distinct.
59fe575 to
e57d4c2
Compare
|
Needs a rebase to |
* feat(tests): Add additional invalid inputs * feat(tests): Add tests for zero gas, 3450 gas * feat(tests): Test precompiles in traditional range, and one outside range * feat(tests): Check return status * feat(tests): Test wrong endianness * fix(tests): Remove brittle storage check
f2619e5 to
6f70107
Compare
|
Another rebase with latest |
6f70107 to
f9ce54a
Compare
* feat(test): port static context static tests to python Port STATICCALL tests with zero and non-zero value to precompiles * feat(test): split into legacy test and high-level API test + parametrize - Inspired by comment: ethereum#1960 (comment) - Split the unreadable bytecode test from the comment where we can parametrize with all precompiles as well as with call_value = [0 and nonzero]. * chore: parametrize CREATE2 from Constantinople (where introduced) * refactor: address comments from PR ethereum#1960 * feat(tests): Add BAL expectations for static call tests >=Amsterdam * refactor(test): Update test name, add to BAL test_cases.md * chore: convert to state_test --------- Co-authored-by: Mario Vega <marioevz@gmail.com>
| ) | ||
| charge_gas(evm, message_call_gas.cost + extend_memory.cost) | ||
|
|
||
| if evm.message.is_static and value != U256(0): |
There was a problem hiding this comment.
This is duplicated. It's higher up in the method in line 407.
| raise WriteInStaticContext | ||
| evm.memory += b"\x00" * extend_memory.expand_by | ||
|
|
||
| # OPERATION |
There was a problem hiding this comment.
If we want to add this, let's be consistent with CALLCODE, STATICCALL, and DELEGATECALL and add this above the evm.memory += ... line. They include memory expansion as part of the # OPERATION.
64417cc to
7d15971
Compare
7d15971 to
b008409
Compare
bfa2cf0
into
ethereum:eips/amsterdam/eip-7708
🗒️ Description
Add a
LOG3like functionality that logs all transfers in the Ethereum system forCALL*andSELFDESTRUCT.🔗 Related Issues or PRs
EIPs#9003
Summary
Update EIP definition in line with comments from Eth Magicians.
Status
✔️ Merged
Proposed Changes
0x00...00(nil)0xff...fe(SYSTEM_ADDRESS)keccak256("Transfer(address,address,uint256)")SELFDESTRUCT@ self logSELFDESTRUCT@ self topic[0]keccak256('Selfdestruct(address,uint256)')EIPs#11127
Summary
Clarify SELFDESTRUCT event
Status
✔️ Merged
Proposed Changes
SELFDESTRUCT@ self topic[1]fromaddress (zero prefixed to fill uint256)contract_address(zero prefixed to fill uint256)SELFDESTRUCT@ self case✅ Checklist
toxchecks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:uvx tox -e statictype(scope):.mkdocs servelocally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.Cute Animal Picture
“Little brown bat” by U.S. Fish and Wildlife Service Headquarters, CC BY 2.0
Footnotes
System Address (
0xff..fe) is being used for now, but this question is still open and needs to be resolved. A shared call for debate might be the best way to deal with this. ↩Too much uncertainty about the number of logs produced. A new EIP could be created if this is deemed a valuable addition ↩
Different enough to warrant it's own EIP. ↩